Skip to content

Use modern CMake's target_* functions to set compiler flags/features#11

Open
lrvdijk wants to merge 10 commits intopmelsted:masterfrom
lrvdijk:master
Open

Use modern CMake's target_* functions to set compiler flags/features#11
lrvdijk wants to merge 10 commits intopmelsted:masterfrom
lrvdijk:master

Conversation

@lrvdijk
Copy link

@lrvdijk lrvdijk commented Feb 11, 2020

Hi all,

Thank you for this great piece of software, it's blazingly fast and memory efficient! The past few days I have been playing around a bit with the API, and trying to integrate it in one of my projects.

In my own C++ projects, I usually include third party depencies as git submodules in a vendor/ directory. Then in my own CMakeLists.txt files, I can call add_subdirectory on those third party dependencies, and all their targets will be available in my own CMakeList.txt too (makes it easy to link them, and configure all include directories etc.)

I did the same thing with Bifrost, however, one of my executables was segfaulting, even though it was a very tiny toy example that hardly did anything. After some debugging, I found that my executable missed the -march=native compiler flag, and therefore the instruction sets of bifrost_static and my executable mismatched, resulting in stack corruption.

The new target_* based API introduced in CMake 3 is supposed to better deal with these kinds of situations. In this pull request I changed the CMakeLists.txt a bit to make use of this new API. This makes it easier to include Bifrost in other (CMake based) projects, by simply including Bifrost using add_directory and linking to bifrost_static or bifrost_dynamic, and ensures that the required compiler flags get propagated to the linked executables too.

I'll admit, I've only minimally changed it, until it fixed my problem 😬 I've not checked the options you seem to use for profiling etc. Anyway hope it's a useful starting point. I've found this guide on modern Cmake helpful: https://cliutils.gitlab.io/modern-cmake/

This is more modern CMake, and makes it easier to include Bifrost
in other CMake projects (as library for example).

One of the big benefits of the new target_* API is that compiler
options like -march=native now get propogated to any executable
linking to bifrost_static of bifrost_dynamic.
@GuillaumeHolley
Copy link
Collaborator

GuillaumeHolley commented Feb 15, 2020

Hi there,

First of all, thank you for your contribution! I am very glad that you like Bifrost so let us know if we can do anything else to help integrate Bifrost into your own project.
Your pull request looks very sound to me and I just want to try it out before merging it ;) I'm a little bit busy those days so no worries, I didn't forget about it, it shouldn't take too long.

Thank you again for your work!

Best,
Guillaume

lrvdijk added 2 commits March 15, 2020 14:49
Should support single and multiple configuration generators.
Profiling currently only supported on GCC.
@lrvdijk
Copy link
Author

lrvdijk commented Mar 15, 2020

I've updated the cmake scripts with improved support for profiling. Setting CMAKE_BUILD_TYPE to Profile should enable the -pg flags when using GCC.

Following guidelines from:
https://stackoverflow.com/questions/24460486/cmake-build-type-is-not-being-used-in-cmakelists-txt/24470998#24470998

@GuillaumeHolley
Copy link
Collaborator

Hey @lrvdijk,

Thank you again for your contribution and your efforts on PyBifrost. It took me some time to get back to you on this PR because I'm developing Bifrost on my free time nowadays.

I have a couple of questions regarding your PR if you don't mind:

  • This line and this line seems to be GCC specific. I could have a look at what Clang proposes for profiling flags but in the meantime, I think either Cmake should fail to produce a Makefile for Clang + Profiling or display an warning message and still compile with "-pg" (Clang will compile with this flag).
  • Is the Debug mode gone (I can see only the Profiling mode)?
  • Lines like this one add the requirement to compile C++ code with C++11. What about the requirement of compiling the C code using C11?
  • I see that the minimum Cmake version requirement is Cmake 3.12. Is it possible to lower that version requirement even further? We will use that version if need be but we've been trying since the beginning to use a cmake version as low as possible to work on older clusters.

Thank you for your time.

lrvdijk added 2 commits April 8, 2020 12:33
- Make sure include directory for bifrost_dynamic is correct when
  installing
- Use the right minimum cmake version
- Give error when trying to enable profiling on a compiler other
  than GCC
@lrvdijk
Copy link
Author

lrvdijk commented Apr 8, 2020

Thanks for your time!

I've updated the CMakeLists.txt a bit more, which should address most of your points! However, I noticed that target_link_options is added in CMake 3.13, so the minimum version actually increased.

Debug mode is not gone, as can be seen on this line:
set(CMAKE_CONFIGURATION_TYPES "${CMAKE_CONFIGURATION_TYPES};Profile")

I append Profile to the existing value of CMAKE_CONFIGURATION_TYPES, which is by default Debug;Release;RelWithDebInfo;MinSizeRel, and with reasonable default compiler flags.
https://cmake.org/cmake/help/latest/variable/CMAKE_BUILD_TYPE.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants